fix(skills): read skill markdown as UTF-8 - #4995
Conversation
|
|
github-actions
Bot
added
area:docs
19c731d to
6e9e48f
Compare
|
@JieZeng777 thanks for your contribution. Please click the CLA assitant button to sign the CLA first. |
|
@WillemJiang Thanks! I’ve signed the CLA, and the I also investigated the failing Skill Review CI. The four reported errors come from existing files in the |
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Solid, minimal fix and a well-targeted regression test. Three things worth addressing: the same locale-dependent read still exists in the sibling skill-creator scripts (notably utils.py: parse_skill_md, which is the SKILL.md reader for run_eval/run_loop/improve_description); the new hard UTF-8 decode turns a non-UTF-8 SKILL.md into an unhandled UnicodeDecodeError instead of the (False, message) contract that package_skill.py relies on; and the new regression test is not executed by any CI workflow, so it only protects local runs.
|
|
||
| # Read and validate frontmatter | ||
| content = skill_md.read_text() | ||
| content = skill_md.read_text(encoding="utf-8") |
There was a problem hiding this comment.
Fix is correct but incomplete against the contract this PR adds to AGENTS.md. parse_skill_md() in skills/public/skill-creator/scripts/utils.py:9 still does (skill_path / "SKILL.md").read_text() with the platform codec, and it is the SKILL.md reader used by run_eval.py:279, run_loop.py:64 and run_loop.py:268, and improve_description.py:213 — so a localized UTF-8 skill still hits the exact UnicodeDecodeError this PR fixes, just through a different script.
Related, same root cause: init_skill.py:230 writes SKILL.md via write_text(skill_content) with no encoding, so on a non-UTF-8 Windows code page skill-creator emits a SKILL.md that already violates the UTF-8 rule. The JSON reads in run_eval.py:272, run_loop.py:261, improve_description.py:208/211 and generate_report.py:314 are locale-dependent too.
|
|
||
| # Read and validate frontmatter | ||
| content = skill_md.read_text() | ||
| content = skill_md.read_text(encoding="utf-8") |
There was a problem hiding this comment.
With a strict UTF-8 decode, a SKILL.md that is not valid UTF-8 now raises UnicodeDecodeError out of validate_skill() instead of returning the (False, message) tuple that every other branch uses. skills/public/skill-creator/scripts/package_skill.py:72 imports and calls this directly (valid, message = validate_skill(skill_path)), so the caller gets a raw traceback instead of the friendly "Validation failed: ..." path.
Suggest wrapping the read in try/except UnicodeDecodeError and returning something like (False, "SKILL.md is not valid UTF-8") — that keeps the tuple contract and gives a diagnosable message (which also points at the encoding mismatch rather than looking like a validator crash).
| return module | ||
|
|
||
|
|
||
| def test_validate_skill_reads_markdown_as_utf8(tmp_path: Path, monkeypatch) -> None: |
There was a problem hiding this comment.
This regression test is not wired into CI. No GitHub Actions workflow and no root Makefile target runs tests/skills/ — CI only executes backend/tests/ (backend-unit-tests.yml, backend-blocking-io-tests.yml, skill-review-ci.yml), and the root Makefile has no test target. So the guard only protects developers who run pytest tests/skills by hand at the repo root.
Worth adding this directory to a workflow (or a root make test) in the same PR, otherwise the encoding regression can quietly come back and the PR's "went red on main, green on branch" property is lost after merge.
|
@JieZeng777, please take a look at the review comments. Now the CI should be fine. |
4 participants
Why
On Windows systems whose default locale is not UTF-8, the skill creator validator reads
SKILL.mdwith the platform default encoding. Localized UTF-8 skills can therefore fail validation withUnicodeDecodeError, even though the skill file itself is valid.What changed
SKILL.mdexplicitly as UTF-8 in the skill creator validator.Surface area
frontend/backend/applanggraph.json, or prompt changedocker/or sandboxed executionskills/backend/pyproject.tomlorfrontend/package.jsonScreenshots / Recording
Not applicable; this change has no UI surface.
Bug fix verification
tests/skills/test_skill_creator_quick_validate.pymainand green on this branch? Yes. Before the fix, the regression test raised the simulated GBKUnicodeDecodeError; after the fix, it passes.Validation
python -m pytest tests/skills -q— 52 passedpython -m ruff check --config backend/ruff.toml tests/skills/test_skill_creator_quick_validate.py— passedpython -m ruff format --check --config backend/ruff.toml tests/skills/test_skill_creator_quick_validate.py— passedpython skills/public/skill-creator/scripts/quick_validate.py skills/public/skill-creator— Skill is validAI assistance
Tool(s) used: Codex
How you used it: Codex helped inspect the Windows locale failure, write the regression test, implement the minimal encoding fix, update documentation, and run validation. I reviewed and understand the resulting change.